Conversation
BREAKING CHANGE: Used pip-compile to create the new file
…with Topic.slug being promoted to PrimaryKey
…lugin in-repo (package already removed from requirements.txt)
…being called before the Apps registry is initialized
…being called before the Apps registry is initialized
- Resolved conflicts in django_topics/migrations/0003_auto_20241121_0757.py - Resolved conflicts in requirements.txt, keeping compiled structure with updated versions - Updated to newer versions: celery[redis]==5.5.2, pymongo==4.13.*, redis==5.2.1 - Added new packages from master: gevent, google-analytics-data, LLM interface - Maintained compiled pip-tools format while incorporating master changes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…go-2-0' into feature/sc-31678/upgrade-to-django-2-0
…-39065/upgrade-to-django-2-x-on-mdl Resolved conflicts in: - reader/templatetags/sefaria_tags.py: Combined import statements and kept static file hash function from HEAD - requirements.txt: Kept Django 2.2 and related packages from merging branch - sefaria/urls.py: Kept minimal URLconf for django-hosts setup - templates/base.html: Combined DJANGO_VARS with contentLang, static_url, and module_mapping This merge brings in: - Django 2.0/2.2 compatibility updates - Fixed circular import in sefaria_tags.py - emailusernames authentication backend - API testing infrastructure (test_api_endpoints.py, analyze_api_errors.py) - Various model and helper updates for Django 2.x 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Sefaria Django application from Django 1.11 to Django 2.2.28. The upgrade includes URL pattern modernization, dependency updates, refactored footnote handling, and the addition of a comprehensive API testing infrastructure. Key changes include migrating from django.conf.urls.url to django.urls.path/re_path, updating deprecated imports, refactoring HTML footnote parsing to use regex-based normalization, and introducing the emailusernames package for email-based authentication.
- Upgraded Django from 1.11 to 2.2.28 with associated dependency updates
- Refactored footnote/itag handling from BeautifulSoup to regex-based FootnoteNormalizer
- Added comprehensive API endpoint testing infrastructure (test_api_endpoints.py, analyze_api_errors.py)
Reviewed changes
Copilot reviewed 26 out of 29 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| test_api_endpoints.py | New comprehensive test script for API endpoints |
| templates/base.html | Added JavaScript variables for contentLang, static_url, and module_mapping |
| sefaria/urls_shared.py | Updated imports and URL patterns from Django 1.x to 2.x syntax |
| sefaria/system/cache.py | Added null check for cached data with decorate_data_with_key |
| sefaria/search.py | Refactored footnote removal to use new FootnoteNormalizer |
| sefaria/model/text.py | Replaced BeautifulSoup-based itag/footnote handling with NormalizerFactory approach |
| sefaria/model/tests/chunk_test.py | Updated test expectations for new footnote handling logic |
| sefaria/model/schema.py | Removed deprecated 'ref_resolver_context_swaps' from optional_param_keys |
| sefaria/helper/tests/normalization_tests.py | Updated tests for new FootnoteNormalizer implementation |
| sefaria/helper/normalization.py | Replaced ITagNormalizer with FootnoteNormalizer using regex-based parsing |
| sefaria/helper/crm/dummy_crm.py | Added get_available_lists method |
| sefaria/gauth/views.py | Updated import from django.core.urlresolvers to django.urls |
| requirements.txt | Updated all dependencies for Django 2.2.28 compatibility |
| reader/templatetags/sefaria_tags.py | Moved imports to function-level scope to avoid circular dependencies |
| emailusernames/* | Added complete emailusernames package for email-based authentication |
| django_topics/migrations/0003_auto_20241121_0757.py | Added comments explaining removed migration operations |
| analyze_api_errors.py | New script to analyze API endpoint errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
templates/base.html
Outdated
| props: {{ propsJSON|default:"null" }}, | ||
| contentLang: "{{ request.contentLang }}", | ||
| inReaderApp: {% if renderStatic %}false{% else %}true{% endif %}, | ||
| static_url: {{ STATIC_PREFIX }}, |
There was a problem hiding this comment.
The STATIC_PREFIX variable is being used without quotes in a JavaScript context. This will cause a JavaScript syntax error because it needs to be a string literal. Change to "{{ STATIC_PREFIX }}" (with quotes).
| static_url: {{ STATIC_PREFIX }}, | |
| static_url: "{{ STATIC_PREFIX }}", |
| # Get an email | ||
| while 1: | ||
| if not email: | ||
| email = raw_input('E-mail address: ') |
There was a problem hiding this comment.
raw_input was removed in Python 3 and replaced with input. Since this codebase is using Python 3 (as evident from #!/usr/bin/env python3 in other files), this should be changed to input('E-mail address: ').
| email = raw_input('E-mail address: ') | |
| email = input('E-mail address: ') |
| @@ -0,0 +1,129 @@ | |||
| from django import forms, VERSION | |||
There was a problem hiding this comment.
Import of 'VERSION' is not used.
| from django import forms, VERSION | |
| from django import forms |
test_api_endpoints.py
Outdated
| import sys | ||
| import json | ||
| import requests | ||
| from urllib.parse import urljoin |
There was a problem hiding this comment.
Import of 'urljoin' is not used.
| from urllib.parse import urljoin |
test_api_endpoints.py
Outdated
| # Import Django test client | ||
| from django.test import Client | ||
| from django.contrib.auth.models import User | ||
| from django.urls import reverse |
There was a problem hiding this comment.
Import of 'reverse' is not used.
| from django.urls import reverse |
| email_as_username or | ||
| ('@' in self.username and old_user.username == old_user.email) | ||
| ) | ||
| except self.__class__.DoesNotExist: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except self.__class__.DoesNotExist: | |
| except self.__class__.DoesNotExist: | |
| # It's expected that the user may not exist yet (e.g., on creation), so we can safely ignore this. |
…ngo 2.0+ In Django 2.0+, monkey patching at module import time can have timing issues with the app loading system. This moves the User model monkey patch to an AppConfig.ready() method, which is the recommended approach for Django 2.0+. Changes: - Add emailusernames/apps.py with EmailUsernamesConfig - Update __init__.py to set default_app_config - Remove module-level monkeypatch_user() call from models.py - Add _patched guard to prevent double-patching - Add unit tests for monkey patch functionality Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…t framework Django's test database creation can overwrite User.__dict__['__init__'] with Model.__init__, removing our monkey patch. The fix detects when this happens and re-applies the patch. Also removes deprecated this_is_the_login_form handling from login template and adds comprehensive tests for the monkey patching behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…o 2 fixes Resolved conflicts in requirements.txt and urls_shared.py. Applied Django 2 compatibility fixes: - decorators.py: explicit login_url for staff_member_required - system/decorators.py: replace render_to_response with render - remote_config/cache.py: catch RuntimeError for pytest-django DB restriction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ll failure Removed duplicate git+https entries for sefaria_llm_interface and elasticsearch-dsl that were already in the pip-compiled section. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mpat langchain-community 0.3.31 requires requests>=2.32.5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Required by the chatbot module, was dropped during merge conflict resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ax error STATIC_PREFIX was rendered unquoted in the DJANGO_VARS JavaScript object, causing "Unexpected token ':'" when FRONT_END_URL contains a full URL (e.g. https://...). This broke all client-side JavaScript. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…itions Removed contentLang, static_url, and module_mapping from DJANGO_VARS that were carried over from the older Django 2.0 branch. Master is the source of truth — these fields are not needed for Django 2. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reverted files that diverged from master without Django 2 justification: - reader/management/commands/runserver.py (debug log) - reader/templatetags/sefaria_tags.py (unrelated import change) - django_topics/migrations/0003 (comment-only change) - Removed analyze_api_errors.py and test_api_endpoints.py (test scripts) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
50 Playwright-based API tests covering texts, search, sheets, topics, calendars, health, profile/auth, and core endpoints. Tests are Django-version-agnostic and can run against any Sefaria instance. Usage: cd api-tests && npm install API_BASE_URL=https://www.sefaria.org npx playwright test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uite Replaced Playwright/TypeScript API tests with pytest/requests equivalents. 49 tests covering texts, search, sheets, topics, calendars, health, profile/auth, and core endpoints. No Django dependency — tests hit HTTP endpoints and can run against any Sefaria instance. Usage: cd api-tests API_BASE_URL=https://www.django-upgrade.cauldron.sefaria.org python -m pytest -v Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Description
Apply the modifications needed to upgrade to Django 2.x to the modularization-branch
Code Changes
See below
Notes